-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QRCodeScanner unit tests #484
base: master
Are you sure you want to change the base?
Conversation
Simplify code in order to understand all QRCodeScanner states; Remove unused QRButton component. The only place where QRCodeScanner is being used is in BottomNav component.
Rename files to .jsx in order to run successfully the component to test.
let backCameraIndex = -1; | ||
if (bestBackCamera) { | ||
backCameraIndex = filteredDevices.findIndex(devices => | ||
devices.device.deviceId === bestBackCamera.device.deviceId); | ||
} | ||
|
||
setCurrentDeviceIndex(backCameraIndex > -1 ? backCameraIndex : 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let backCameraIndex = -1; | |
if (bestBackCamera) { | |
backCameraIndex = filteredDevices.findIndex(devices => | |
devices.device.deviceId === bestBackCamera.device.deviceId); | |
} | |
setCurrentDeviceIndex(backCameraIndex > -1 ? backCameraIndex : 0); | |
setCurrentDeviceIndex(Math.max(0, filteredDevices.findIndex(({ device: { deviceId } }) => deviceId === bestBackCamera?.device.deviceId))); |
|
||
useEffect(() => { | ||
navigator.mediaDevices.getUserMedia({ video: true }) | ||
.then(stream => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use async/await with try/catch everywhere instead of then/catch
@@ -0,0 +1,270 @@ | |||
import React, { useState, useEffect, useRef } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When refactoring or adding code, I think we should use always use typescript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. We usually refactor with TS. We'll definitely look into that. Do you think it is suitable to make that change in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessarily in this PR
setQrDetected(true); | ||
// Redirect to the URL found in the QR code | ||
const scannedUrl = result.data; | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this timeout have any effect, since there is a shorter timeout for switching the window.location.href defined right after?
}; | ||
|
||
const onUserMedia = () => { | ||
if (webcamRef.current && webcamRef.current.video) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is more readable if you return early on the opposite condition.
No description provided.